-
Notifications
You must be signed in to change notification settings - Fork 672
[WIP]: Create a Plugin System for Lima #3573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
77877e0
to
7a082d2
Compare
cmd/limactl/start.go
Outdated
@@ -54,6 +56,9 @@ $ limactl create --set='.cpus = 2 | .memory = "2GiB"' | |||
To see the template list: | |||
$ limactl create --list-templates | |||
|
|||
To see the drivers list: | |||
$ limactl create --list-drivers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example currently just focuses on templates, not on drivers.
pkg/driver/vz/vz_driver_others.go
Outdated
@@ -0,0 +1,118 @@ | |||
//go:build !darwin || no_vz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package shouldn't be compiled at all on non-Darwin
3f1de89
to
e53a368
Compare
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…plugin related functions Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…rnal drivers Signed-off-by: Ansuman Sahoo <[email protected]>
…internal plugins Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…rface Signed-off-by: Ansuman Sahoo <[email protected]>
…ement common error Signed-off-by: Ansuman Sahoo <[email protected]>
e53a368
to
4630452
Compare
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…function Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
func (d *DriverClient) Initialize(ctx context.Context) error { | ||
d.logger.Debug("Initializing driver instance") | ||
|
||
connCtx, cancel := context.WithTimeout(ctx, 5*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeout should be specified in the caller
for { | ||
n, err := conn.Read(buf) | ||
if err != nil { | ||
if err == io.EOF { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Is
is more robust
…ocess Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…in SetConfig() Signed-off-by: Ansuman Sahoo <[email protected]>
605e45e
to
2a82bf3
Compare
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
pkg/driver/qemu/cmd/main.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be cmd/<EXECUTABLE NAME>/main.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefile has to be updated to compile this by default.
How to test this ? Tried |
The driver discovery through the directory is not working rn. I will work on it once I fix the GuestAgentConn() function. For now, you can test the driver by setting the executable driver's path to |
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
…socket Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
@@ -0,0 +1,9 @@ | |||
//go:build darwin && !no_vz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to avoid double-negation, when possible:
//go:build darwin && !no_vz | |
//go:build darwin && driver_vz |
Then the Makefile
will just have a DRIVER_TAGS=driver_qemu,driver_vz,driver_wsl
setting that the user can override to only include the desired driver(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also go:build darwin
is already implied by the filename
func (d *BaseDriver) Stop(_ context.Context) error { | ||
return nil | ||
ChangeDisplayPassword(ctx context.Context, password string) error | ||
GetDisplayConnection(ctx context.Context) (string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Go "getters" don't use a Get
prefix. You know it is a getter because the method name doesn't include an action word (verb).
GetDisplayConnection(ctx context.Context) (string, error) | |
DisplayConnection(ctx context.Context) (string, error) |
func (d *BaseDriver) ApplySnapshot(_ context.Context, _ string) error { | ||
return errors.New("unimplemented") | ||
} | ||
GetInfo() Info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to comment on additional setters; they all should drop the Get
prefix.
GetInfo() Info | |
Info() Info |
func (d *BaseDriver) DeleteSnapshot(_ context.Context, _ string) error { | ||
return errors.New("unimplemented") | ||
// SetConfig sets the configuration for the instance. | ||
SetConfig(inst *store.Instance, sshLocalPort int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense to have a separate type (like the Kubernetes Completed*
types):
type ConfiguredDriver struct {
Driver
}
Then you could have a function to turn a Driver
into a ConfiguredDriver
:
SetConfig(inst *store.Instance, sshLocalPort int) | |
Configure(inst *store.Instance, sshLocalPort int) ConfiguredDriver |
And all the functions/methods that would only work with configured driver would only accept that type instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move this to cmd/lima-driver-qemu
at the top level. There should be no main
packages with a main
function inside the pkg
tree.
VSockPort int | ||
VirtioPort string | ||
|
||
DriverType driver.DriverType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like a failure of the abstraction if the driver needs to know if it is an internal or external driver. I was hoping that this could be entirely handled by the remoting layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GuestAgentConn needs to know this, at least for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this now. I hope we can fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to deal with this would be to have both a GuestAgentConn()
and ExternalGuestAgentConn()
method on the driver, and the driverserver.GuestAgentConn()
method in pkg/driver/external/server/methods.go
would call s.driver.ExternalGuestAgentConn()
instead of the internal version.
Maybe call it GuestAgentConnExternal()
so both have the same prefix, idk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my earlier feedback from today was supposed to be part of this, but I continued to press the wrong button.
@@ -0,0 +1,18 @@ | |||
//go:build (darwin && amd64) || (darwin && arm64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not just checking for darwin
?
//go:build (darwin && amd64) || (darwin && arm64) | |
//go:build darwin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some inconsistency in the go:build
line between the files in this directory. E.g. errors_darwin.go
has
/go:build darwin && !no_vz
It looks like including/excluding the driver is controlled by importing pkg/driver/vz/register
into main
, so that file is really the only one that needs the go:build
line. All the rest are not referenced and don't need it.
If we want to keep it for some kind of "belts & suspender" reasons, then all the files should have it.
I also think that we should rename all the file to remove the _darwin
suffix from the filenames. It is no longer needed because the files will not be referenced on any other OS. Only the _arm64
suffix on the Rosetta code needs to stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is no longer needed because the files will not be referenced on any other OS.
Still needed to allow running golangci-lint run ./...
, govulncheck ./...
, etc. on non-macOS ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needed to allow running
golangci-lint run ./...
,govulncheck ./...
, etc. on non-macOS ?
Yeah, probably true. But then it can be removed from the go:build
line.
if *limaDriver == limayaml.WSL2 { | ||
return wsl2.New(base) | ||
// CreateTargetDriverInstance creates the appropriate driver for an instance. | ||
func CreateTargetDriverInstance(inst *store.Instance, sshLocalPort int) (driver.Driver, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are overloading the word instance
here, for drivers and machines.
I think my previous suggestion to use a different type for DriverInstance
would work well here:
func CreateTargetDriverInstance(inst *store.Instance, sshLocalPort int) (driver.Driver, error) { | |
func CreateConfiguredDriver(inst *store.Instance, sshLocalPort int) (driver.ConfiguredDriver, error) { |
driver.SetConfig(inst, sshLocalPort) | ||
|
||
return driver, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driver.SetConfig(inst, sshLocalPort) | |
return driver, nil | |
return driver.Configure(inst, sshLocalPort) |
if err != nil { | ||
return err | ||
} | ||
stdDriverDir := filepath.Join(homeDir, ".local", "libexec", "lima", "drivers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was going to be /usr/local/libexec/lima
and not a user-owned directory. We don't store anything except LIMA_HOME
under the user's home directory.
|
||
info, err := os.Stat(path) | ||
if err != nil { | ||
logrus.Warnf("Error accessing driver path %s: %v", path, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency we should always quote filenames. Especially if they are user-provided and may contain whitespace. But let's just always do it:
logrus.Warnf("Error accessing driver path %s: %v", path, err) | |
logrus.Warnf("Error accessing driver path %q: %v", path, err) |
func (r *Registry) registerDriverFile(path string) { | ||
base := filepath.Base(path) | ||
if !strings.HasPrefix(base, "lima-driver-") { | ||
fmt.Printf("Skipping %s: does not start with 'lima-driver-'\n", base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is debugging code; we don't want to print anything for ignored files.
} | ||
|
||
name := strings.TrimPrefix(base, "lima-driver-") | ||
name = strings.TrimSuffix(name, filepath.Ext(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fine; we don't need to allow .
in driver names. Otherwise I would have said we only strip .exe
suffix on Windows, but otherwise allow any characters.
errorsAsStrings := make([]string, len(inst.Errors)) | ||
for i, err := range inst.Errors { | ||
if err != nil { | ||
errorsAsStrings[i] = err.Error() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is going to work properly. E.g. if the error is later processed again with errors.Is()
, then the result will always be false
because we are not preserving the type.
Not sure if that is important, but it would require some reflection magic to make it work.
Important
This PR is only for preview purposes and not meant to be merged. Later, the commits can be cherry-picked and raised as separate small PRs.
This PR creates a Plugin System for drivers which can be embedded in the main binary or built as separate binaries, which then communicate with the main binary using gRPC. This work is done as part of my GSoC '25 task.